-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add for-loops to html!
#3498
base: master
Are you sure you want to change the base?
Add for-loops to html!
#3498
Conversation
Thanks for the pull request, however, I do not think an ordinary for-loop should be supported. What would happen for the following expression? html! {
for x in 0..10 {
<span>{break x}</span>
}
} |
Benchmark - coreYew Master
Pull Request
|
Visit the preview URL for this PR (updated for commit a9a1c49): https://yew-rs--pr3498-add-html-for-jj737upv.web.app (expires Mon, 11 Dec 2023 15:29:52 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 |
Benchmark - SSRYew Master
Pull Request
|
Size Comparison
✅ None of the examples has changed their size significantly. |
Fixed it, now an error is raised when trying to break out of a for loop or continue it |
If we block break and continue, then code like the following will no longer work, which should be allowed. #[function_component]
fn Comp() -> Html {
html! {
for i in 0..5 {
<div>
{{
loop {
let a = rand_number();
if a % 2 == 0 {
break a;
}
}
}}
</div>
}
}
} I am fine with asking users to write the following: let children = (0..5).map(
html! { <span key={x}>{x}</span> }
);
html! { for x } There are 2 reasons:
The current key requirement can be summarised in 1 sentence: Everything defined in (I consider current |
I'm fine with supporting loops. This was discussed on discord as well. I agree that breaks and continues can be problematic but we could disallow those. These loops can be a for-loop over an iterator that always yields a VNode. This allows us to also lint for presence of keys at compile time.
I also agree, however it's not possible to |
I am specifically referring to the
|
html! {
for i in fragments {
{i.to_html()}
}
} I think it would be kind of difficult to lint this at compile time. |
Which is why we can just not care about the peculiar case of nested loops shown above, it'll be rejected anyway.
Not necessarily, unlike Another argument that can be made in favour of the new for-loops are the possible optimisations and checks, of course one can always just {for iter.into_iter().map(|x| html!{...})} Yet, even ignoring how unreadable it can get, which is subjective in a way, calling |
We need to ensure that our implementation is sound. If it is a valid Rust expression, we should accept it. If we wish to reject complex expressions, it needs to be rejected based on complexity and not a limitation introduced by blocking expressions.
If you reconcile a for-generated VList of 4 items into 5 items, the renderer will not understand which one you are trying to remove. (That's why you need to key the iterator expression.) The keying requirement always presents for VList items with a non-fixed length. The syntax proposed in this pull request ( You can read the React documentation, which explains the keying requirement for Lists in detail: https://react.dev/learn/rendering-lists The bug for
Optimisation is also possible for iterators-based implementations with helpers like |
…f a for loop, added a check for duplicate keys, added keyed state compile-time inference
Added a check to outlaw the following cases: html!{
for i in 0 .. 10 {
<div key="duplicate" />
}
} Outlaws the most obvious cases of incorrect keying: when a key is a complex path (i.e. Also added an optimisation which allows for avoiding calling Regarding |
There's some problem with the benchmarks again, some file missing... |
Description
{for x}
, i.e. the following is no longer valid:To disambiguate it and the new for loops, it'll be required to wrap the expression in braces like so:
Checklist